Conversation
…riment logging This commit refactors the `experiment.py`, `job_manager.py`, `model.py`, and related files to replace instances of `repeat_id` with `run_id`. This change enhances clarity and consistency across the codebase, particularly in logging and data collection. Additionally, it introduces backward compatibility warnings for deprecated `repeat_id` usage in the `ExperimentManager` class, ensuring a smoother transition for users. Tests are updated to verify that `run_id` is correctly propagated in outputs.
📝 WalkthroughWalkthroughThis PR consistently renames the run-flow identifier from Changes
Sequence Diagram(s)(Skipped — changes are primarily parameter renames, data injection, and doc updates; control flow not substantially altered to require sequence diagrams.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
abses/core/job_manager.py (1)
80-84: Consider updating index names for consistency with the run_id terminology.The MultiIndex still uses
"repeat_id"as a name, which is inconsistent with the broader rename torun_idthroughout the PR. This means:
- When
get_datasets()callsreset_index(), it creates a"repeat_id"column from the index- The deprecation warning at lines 147-152 will fire on every call, even for new code
- Users will see both
repeat_idandrun_idcolumns, creating confusionConsider updating line 83 to use
"run_id"instead:- names=["job_id", "repeat_id"] + names=["job_id", "run_id"]This would make the index naming consistent with the documentation (line 106) and the rest of the codebase.
🤖 Fix all issues with AI agents
In @abses/core/experiment.py:
- Around line 373-399: Remove the entire commented-out block that defines the
old _get_logging_mode and _update_log_config methods (the 27-line commented code
in experiment.py); delete these commented lines so only active code remains, and
if historical reference is needed, record the previous implementation in a
commit message or issue rather than keeping it inline.
In @abses/utils/log_config.py:
- Around line 236-241: Update the docstring parameter description to use "Run
ID" instead of "Repeat ID" so it matches the actual parameter name `run_id`;
find the function or method whose Args block contains `run_id` (the docstring
lines listing "Repeat ID for the current run (1-indexed)") and replace that
wording with "Run ID for the current run (1-indexed)" to keep terminology
consistent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
abses/core/experiment.pyabses/core/job_manager.pyabses/core/model.pyabses/utils/datacollector.pyabses/utils/log_config.pyabses/utils/logging.pydocs/home/configuration_schema.mdtests/api/test_datacollection.pytests/utils/test_logging.py
🧰 Additional context used
🧬 Code graph analysis (6)
abses/utils/log_config.py (3)
abses/core/model.py (2)
run_id(414-419)outpath(170-179)abses/core/experiment.py (1)
outpath(277-281)abses/core/protocols.py (2)
outpath(236-236)outpath(278-278)
tests/utils/test_logging.py (2)
abses/core/model.py (2)
outpath(170-179)run_id(414-419)abses/utils/log_config.py (1)
determine_log_file_path(228-269)
abses/utils/logging.py (1)
abses/core/model.py (1)
run_id(414-419)
tests/api/test_datacollection.py (3)
tests/conftest.py (1)
test_config(56-60)abses/core/model.py (2)
run_id(414-419)MainModel(73-568)abses/utils/datacollector.py (3)
get_model_vars_dataframe(218-231)get_agent_vars_dataframe(233-248)get_final_vars_report(250-263)
abses/core/experiment.py (5)
abses/core/model.py (1)
run_id(414-419)abses/core/base_module.py (2)
model(70-76)model(79-85)abses/core/protocols.py (3)
model(200-200)random(331-333)random(443-443)abses/space/move.py (1)
random(234-247)abses/core/job_manager.py (1)
hooks(71-77)
abses/utils/datacollector.py (1)
abses/core/model.py (1)
run_id(414-419)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Tests 3.13 on windows-latest
- GitHub Check: Tests 3.12 on windows-latest
- GitHub Check: Tests 3.11 on windows-latest
- GitHub Check: Tests 3.13 on macos-latest
- GitHub Check: Tests 3.11 on ubuntu-latest
- GitHub Check: Tests 3.12 on macos-latest
- GitHub Check: Tests 3.11 on macos-latest
- GitHub Check: Tests 3.13 on ubuntu-latest
- GitHub Check: Tests 3.12 on ubuntu-latest
- GitHub Check: nbmake
- GitHub Check: nbmake
🔇 Additional comments (25)
abses/core/job_manager.py (2)
10-10: LGTM: Necessary import for deprecation warnings.The
warningsmodule is correctly imported to support the backward compatibility deprecation path.
106-106: LGTM: Documentation updated for consistency.The docstring correctly reflects the new
run_idterminology.tests/api/test_datacollection.py (1)
94-127: LGTM: Comprehensive test for run_id propagation.The new test thoroughly verifies that
run_idis correctly propagated to all data collection outputs:
- Model-level data (
get_model_vars_dataframe)- Agent-level data (
get_agent_vars_dataframe)- Final report (
get_final_vars_report)The test correctly asserts both the presence and value of
run_id, ensuring the feature works end-to-end.abses/core/model.py (2)
143-145: LGTM: Correct run_id propagation to datacollector.The
run_idparameter is properly passed toABSESpyDataCollector, enabling it to injectrun_idinto collected data as verified by the test intests/api/test_datacollection.py.
352-352: LGTM: Correct run_id propagation to logger setup.The
run_idis properly passed tosetup_model_logger, which uses it for log file path determination in different logging modes (once/separate/merge).tests/utils/test_logging.py (5)
179-210: LGTM: Test calls updated for run_id parameter.The test calls to
determine_log_file_pathare correctly updated to use therun_idparameter, maintaining test coverage for different logging modes (once, separate, merge).
347-366: LGTM: setup_model_logger calls updated to use run_id.The test correctly passes
run_id=1tosetup_model_logger, aligning with the renamed parameter.
470-470: LGTM: Consistent run_id usage in integration tests.The integration test properly uses
run_id=1when callingsetup_model_logger.
488-514: LGTM: Comprehensive logging mode tests updated.The logging mode tests are correctly updated to pass
run_idtodetermine_log_file_pathfor all three modes (once, separate, merge).
588-588: LGTM: User module logger test updated.The test correctly uses
run_id=1in thesetup_model_loggercall.abses/utils/logging.py (2)
89-99: LGTM: Function signature and implementation updated for run_id.The
log_repeat_separatorfunction is correctly updated to userun_idin its signature, docstring, and implementation.
114-159: LGTM: setup_model_logger signature updated for run_id.The function signature, docstring, and internal call to
determine_log_file_pathare all correctly updated to userun_idinstead ofrepeat_id, maintaining consistency across the logging module.abses/utils/log_config.py (2)
228-233: LGTM! Parameter successfully renamed fromrepeat_idtorun_id.The function signature has been correctly updated to use
run_idthroughout, maintaining backward compatibility in the logic whereNoneis treated as the first run.
251-270: LGTM! Logic correctly updated to userun_id.All conditional checks and file name generation have been properly updated to use
run_idinstead ofrepeat_id, maintaining the original behavior.abses/core/experiment.py (5)
107-141: LGTM! Core run flow successfully updated to userun_id.The function correctly unpacks
run_idfrom the key tuple, passes it to model initialization, and propagates it to hooks. This aligns with the PR objective of standardizing onrun_idterminology.
401-424: LGTM! Seed generation correctly updated to userun_id.The
_get_seedmethod signature, documentation, and implementation have been properly updated to userun_id. The seed calculation formulaself._base_seed + job_id * 1000 + run_idensures unique seeds for each job-run combination.
433-453: LGTM! Log file path resolution updated to userun_id.The method signature, docstring, and call to
determine_log_file_pathhave all been correctly updated to userun_idinstead ofrepeat_id.
484-587: LGTM! Batch execution loop correctly updated to userun_id.All references in the batch execution logic have been systematically updated:
- Loop variable renamed to
run_id- Progress messages reference
run_id- Log path resolution uses
run_id- Model execution keys constructed with
run_id- Seed generation calls use
run_idThe changes maintain consistency throughout the execution flow.
643-664: LGTM! Hook invocation updated to userun_id.The helper function signature, docstring, and parameter inspection logic have been properly updated to check for and pass
run_idinstead ofrepeat_id.abses/utils/datacollector.py (5)
115-131: LGTM! Constructor updated to accept and storerun_id.The
run_idparameter has been properly added to the constructor signature, documented, and stored as an instance variable for use in data collection methods.
168-174: LGTM! Helper method for injectingrun_idinto data.The
_add_run_id_to_datamethod provides a clean, reusable way to addrun_idto both DataFrame and dictionary data structures when present. This follows the DRY principle effectively.
218-232: LGTM! Model variables DataFrame now includesrun_id.The method correctly augments the model variables DataFrame with
run_idwhen available, supporting the PR's goal of including run identifiers in collected data.
233-249: LGTM! Agent variables DataFrame now includesrun_id.The method has been properly updated to:
- Concatenate agent records into a DataFrame
- Handle the case of missing records with an empty DataFrame
- Augment the result with
run_idvia the helper method
250-264: Verify log level change is intentional.Line 257 changes the log level from
warningtoinfowhen no final reporters are defined. This change appears unrelated to therun_idrefactoring. Was this intentional, or should it remain a warning?Context: The message "No final reporters have been defined" might be informational rather than a problem, so
infolevel could be appropriate if users commonly run experiments without final reporters.docs/home/configuration_schema.md (1)
444-590: No duplication found. The "Logging Configuration" section appears only once at line 444; the subsequent section "Parameter Sweeps" begins at line 592. No action needed.Likely an incorrect or invalid review comment.
…ce run_id usage This commit removes commented-out code related to the deprecated `repeat_id` in the `experiment.py` file and updates the `job_manager.py` and `log_config.py` files to consistently use `run_id`. It introduces backward compatibility warnings for any legacy usage of `repeat_id` in the `ExperimentManager` class, ensuring users are informed of the transition. These changes improve clarity and consistency in logging and data handling across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
abses/core/experiment.py (3)
373-395: Seed generation logic correctly updated to use run_id.The seed generation function properly incorporates
run_idinto the random seed computation, ensuring unique seeds for each run within a job.📝 Optional: Update Chinese docstring for consistency
For complete consistency, the Chinese docstring at line 383 could be updated from "重复实验的ID" (repeat experiment ID) to "运行ID" (run ID) to match the parameter name change.
Args: - run_id: 重复实验的ID + run_id: 运行ID
406-425: Log file path resolution correctly updated to use run_id.The method properly passes
run_idto the downstream log path determination function.📝 Optional: Update docstring terminology
For consistency with the parameter rename, update the docstring at line 412:
Args: log_name: Base log file name. - run_id: Repeat ID (1-indexed). + run_id: Run ID (1-indexed). logging_mode: Logging mode.
619-635: Hook helper function correctly updated to use run_id.The function properly inspects hook signatures for
run_idparameter and passes it accordingly, enabling hooks to receive run identifiers.📝 Optional: Update Chinese docstring for consistency
For complete consistency, the Chinese docstring at line 627 could be updated from "重复实验ID" (repeat experiment ID) to "运行ID" (run ID):
job_id: 可选的任务ID - run_id: 可选的重复实验ID + run_id: 可选的运行ID
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
abses/core/experiment.pyabses/core/job_manager.pyabses/utils/log_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- abses/core/job_manager.py
🧰 Additional context used
🧬 Code graph analysis (1)
abses/utils/log_config.py (1)
abses/core/model.py (2)
run_id(414-419)outpath(170-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Tests 3.13 on ubuntu-latest
- GitHub Check: Tests 3.12 on windows-latest
- GitHub Check: Tests 3.11 on windows-latest
- GitHub Check: Tests 3.13 on macos-latest
- GitHub Check: Tests 3.12 on ubuntu-latest
- GitHub Check: Tests 3.11 on ubuntu-latest
- GitHub Check: Tests 3.12 on macos-latest
- GitHub Check: Tests 3.13 on windows-latest
- GitHub Check: Tests 3.11 on macos-latest
- GitHub Check: nbmake
- GitHub Check: nbmake
🔇 Additional comments (6)
abses/core/experiment.py (4)
125-129: LGTM! Key unpacking and model initialization updated correctly.The tuple unpacking and model initialization correctly use
run_idterminology, maintaining consistency with the model's API that now acceptsrun_idas a parameter.
138-138: Hook invocation correctly updated to pass run_id.The hook invocation properly propagates
run_idto the helper function, ensuring hooks receive the correct run identifier.
493-524: LGTM! Sequential batch execution correctly updated.The loop variable and all related function calls properly use
run_idterminology throughout the sequential execution path, maintaining consistency with the refactoring objective.
539-549: Parallel execution path correctly updated to use run_id.The parallel execution using joblib properly constructs keys and seeds with
run_id, maintaining consistency with the sequential execution path.abses/utils/log_config.py (2)
228-240: LGTM! Function signature and docstring correctly updated.The function signature and docstring properly reflect the
run_idterminology, addressing the previous review feedback about consistent parameter naming.
253-269: Logic branches correctly updated to use run_id.All conditional branches properly use
run_idfor determining log file paths across different logging modes, maintaining the function's intended behavior while using consistent terminology.
1 participant
This commit refactors the
experiment.py,job_manager.py,model.py, and related files to replace instances ofrepeat_idwithrun_id. This change enhances clarity and consistency across the codebase, particularly in logging and data collection. Additionally, it introduces backward compatibility warnings for deprecatedrepeat_idusage in theExperimentManagerclass, ensuring a smoother transition for users. Tests are updated to verify thatrun_idis correctly propagated in outputs.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
API
Tests
✏️ Tip: You can customize this high-level summary in your review settings.